fix(#1877): close token-rotation race on restart — Option A+Option B - #1921
Conversation
|
[retarget-bot] This PR was opened against Why: per SHARED_RULES rule 8, all feature work targets What changed: just the base branch — no code change. CI will re-run against If this PR is the CEO's staging→main promotion: the Action skipped you (only bot-authored PRs are retargeted). If you see this comment on your CEO PR, that's a bug — please tag @HongmingWang-Rabbit. |
Review: fix(#1877) — Token Rotation Race (Option A + Option B)Canvas/FE changes reviewed. Canvas changes — overlap warningPR #1921 touches two canvas files also in PR #1920 (which is queued to merge to main): ApprovalBanner.tsx — adds ContextMenu.keyboard.test.tsx — adds Recommendation: Whoever resolves the sync conflict should pick ONE assertion style:
Both are valid. Suggest using Token rotation fix — architecture reviewOption B (platform): writing Option A (runtime): LGTM overall — the two-layer defense (platform-level elimination + runtime self-heal) is the right pattern here. |
Review — PR #1921 ✅ LGTM (with one note)fix(#1877): close token-rotation race on restart — Option A+Option B What this PR doesFixes the token-rotation race condition described in issue #1877: after platform token rotation, workspaces that restart before the rotation completes load the old Quality assessmentCI.yml — workflow_dispatch trigger: ✅ Adds manual invocation to CI with a canvas/Dockerfile — non-root without fixed UID: ✅ OrgsPage accessibility fixes: ✅ Several good a11y improvements:
workspace_restart.go — org-templates fallback: ✅ When a2a_tools tests — workspace_id in params + RBAC tests: ✅ One note (non-blocking)The RBAC test pattern ( VerdictAPPROVE (comment). Option A+Option B token fix appears sound. Canvas Dockerfile and OrgsPage a11y improvements are solid. Ready to merge. |
1c582ae to
d4bd3e9
Compare
✅ Rebased clean onto staging HEADBranch rebuilt: cherry-picked the 2 core fix commits ( What's in this PR (4 files, 87 lines):
CI will re-run on new HEAD. /cc @airenostars for review once CI is green. |
![molecule-ai[bot]](https://avatars.githubusercontent.com/u/275794187?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #1921 Review — Token Rotation Race Fix ⚠️ COMMENT
CI: All checks green ✅ (Platform/Go, Python Lint, CodeQL all pass)
Code Quality: Excellent
Option A + B dual-layer fix is well-designed:
- Option B (platform): Pre-writing token to volume before ContainerStart eliminates the race window at the root cause. Best-effort with nil check is correct — degrades gracefully if provisioner unavailable.
- Option A (runtime): Self-healing heartbeat retry catches any residual races. Pattern is sound.
workspace_provision.go: Clean, well-commented, best-effort with proper fallback.
platform_auth.py: is minimal and correct — just clears in-process cache.
heartbeat.py: 401 detection + retry logic is clean. Retry counter correctly reset on success.
⚠️ BLOCKING Security Flag —
provisioner.go: The alpine throwaway container runs as root with access to all named Docker volumes via the bind mount. While ephemeral (immediately removed), this gives a container with root privileges broad filesystem access on the Docker host.
This is a privilege escalation concern that should be reviewed by Core-Security before merge:
- Can the alpine container be replaced with a non-root container, or a read-only approach?
- Is this consistent with the provisioner's existing security posture (does it already run privileged containers)?
- If the provisioner is already privileged, this may be acceptable — but it should be explicitly confirmed.
Recommend: Block merge pending Core-Security sign-off on the container privilege level.
Non-blocking note
PR description mentions E2E smoke test as acceptance criteria ("restart 5×, converges within 60s"). No new E2E tests in this PR — confirm those manual tests pass before closing issue #1877.
Overall: Technically sound, CI green, but needs Core-Security review on the Docker privilege pattern before merge.
There was a problem hiding this comment.
PR #1921 Review — Token Rotation Race Fix ⚠️ COMMENT
CI: All checks green ✅ (Platform/Go, Python Lint, CodeQL all pass)
Code Quality: Excellent
Option A + B dual-layer fix is well-designed:
- Option B (platform): Pre-writing token to volume before ContainerStart eliminates the race window at the root cause. Best-effort with nil check is correct — degrades gracefully if provisioner unavailable.
- Option A (runtime): Self-healing heartbeat retry catches any residual races. Pattern is sound.
workspace_provision.go: Clean, well-commented, best-effort with proper fallback.
platform_auth.py: refresh_cache() is minimal and correct — just clears in-process cache.
heartbeat.py: 401 detection + retry logic is clean. Retry counter correctly reset on success.
⚠️ BLOCKING Security Flag — WriteAuthTokenToVolume()
provisioner.go: The alpine throwaway container runs as root with access to all named Docker volumes via the bind mount. While ephemeral (immediately removed), this gives a container with root privileges broad filesystem access on the Docker host.
This is a privilege escalation concern that should be reviewed by Core-Security before merge:
- Can the alpine container be replaced with a non-root container, or a read-only approach?
- Is this consistent with the provisioner's existing security posture (does it already run privileged containers)?
- If the provisioner is already privileged, this may be acceptable — but it should be explicitly confirmed.
Recommend: Block merge pending Core-Security sign-off on the WriteAuthTokenToVolume() container privilege level.
Non-blocking note
PR description mentions E2E smoke test as acceptance criteria ("restart 5x, converges within 60s"). No new E2E tests in this PR — confirm those manual tests pass before closing issue #1877.
Overall: Technically sound, CI green, but needs Core-Security review on the Docker privilege pattern before merge.
…combined Platform side (Option B): - provisioner.go: add WriteAuthTokenToVolume() — writes .auth_token to the Docker named volume BEFORE ContainerStart using a throwaway alpine container, eliminating the race window where a restarted container could read a stale token before WriteFilesToContainer writes the new one. - workspace_provision.go: call WriteAuthTokenToVolume() in issueAndInjectToken as a best-effort pre-write before the container starts. Runtime side (Option A): - heartbeat.py: on HTTPStatusError 401 from /registry/heartbeat, call refresh_cache() to force re-read of /configs/.auth_token from disk, then retry the heartbeat once. Fall through to normal failure tracking if the retry also fails. - platform_auth.py: add refresh_cache() which discards the in-process _cached_token and calls get_token() to re-read from disk. Together these eliminate the >1 consecutive 401 window described in issue #1877. Pre-write (B) is the primary fix; runtime retry (A) is the self-healing fallback for any residual race. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix panic in TestIssueAndInjectToken_HappyPath where h.provisioner is nil (the handler was created without a real provisioner in unit tests). Add nil guard so the pre-write step is skipped gracefully — token is still injected into ConfigFiles as before, and the runtime-side 401 retry handles any race. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
d4bd3e9 to
88c9298
Compare
HongmingWang-Rabbit
left a comment
HongmingWang-Rabbit
left a comment
There was a problem hiding this comment.
Counter-review — clearing the bot's security flag
I read the molecule-ai bot's earlier review and went into the code to verify the "alpine container has access to all named Docker volumes" claim. That claim is incorrect.
`WriteAuthTokenToVolume` binds a single specific volume:
```go
Binds: []string{volName + ":/vol"},
```
where `volName = ConfigVolumeName(workspaceID)` — the target workspace's config volume only. No access to other volumes, no host FS bind, no `--privileged`.
This is the same pattern the existing provisioner already uses for `ReadFromVolume` (provisioner.go:707) and the `file-exists` check (provisioner.go:922), both with `alpine` throwaways against specific volumes. The only difference is `:ro` → rw (can't be read-only when writing).
Real notes (non-blocking)
- `Env: []string{"TOKEN=" + token}` is correct — env not argv, so token doesn't appear in `ps` / host logs.
- `alpine` with no pinned tag — worth a follow-up to pin across all three call sites (existing + new), not unique to this PR.
- Minor: `printf '%s' $TOKEN` inside `sh -c` is fine (no word-splitting since token is opaque bytes in env var), but `"$TOKEN"` would be belt-and-suspenders.
Verdict
Approving the security posture. Waiting on Platform (Go) CI to finish the rebase run. Will merge if green.
Rebase of PR #1921 onto post-#1885 staging: the token-rotation race fix (Options A+B) was based on staging before PR #1885 merged. Add the same ValidateAnyToken fallback that was in #1885 so internal A2A calls with org-level shared tokens still pass through to canCommunicateCheck. Security: org tokens pass hierarchy check; workspace-bound tokens get ValidateToken strict check; invalid tokens get 401. No regression.
1 participant
Fix: Token Rotation Race on Restart (Issue #1877)
Root cause
After
issueAndInjectTokenrevokes old + issues new token,WriteFilesToContainerwrites.auth_tokento/configsafterContainerStart. If the runtime boots and caches the token from disk before that write lands, it holds the old/stale token → 401 heartbeat loop.Fix: Option A + Option B combined
Platform (Option B — primary fix):
provisioner.go:WriteAuthTokenToVolume()— writes.auth_tokento the Docker named volume beforeContainerStartusing a throwaway alpine container, eliminating the race window.workspace_provision.go: callWriteAuthTokenToVolume()inissueAndInjectToken()as a best-effort pre-write.Runtime (Option A — self-healing fallback):
heartbeat.py: on HTTPStatusError 401 from/registry/heartbeat, callrefresh_cache()to force re-read of/configs/.auth_tokenfrom disk, then retry once.platform_auth.py: addrefresh_cache()which discards in-process_cached_tokenand re-reads from disk.Acceptance criteria
/registry/heartbeatin normal operationonlinewithin 60sFiles changed
workspace-server/internal/provisioner/provisioner.go— +34 lines, newWriteAuthTokenToVolume()methodworkspace-server/internal/handlers/workspace_provision.go— +6 lines, callWriteAuthTokenToVolume()after token issuanceworkspace/heartbeat.py— +14 lines, 401 detection + refresh + retryworkspace/platform_auth.py— +9 lines,refresh_cache()function🤖 Generated with Claude Code